tests/verify_backup_permissions.sh (3)

2-9: set -e without a cleanup trap leaves temp dirs behind on unexpected failures.

Any unhandled non-zero exit between the mktemp and the final rm -rf "$TEST_DIR" will bypass cleanup.

♻️ Add a trap for reliable cleanup

 set -e
+
+# Ensure cleanup on exit (normal or error)
+cleanup() { rm -rf "$TEST_DIR"; }
+trap cleanup EXIT

You can then remove the manual rm -rf "$TEST_DIR" lines inside the || { ... } block and the [[ ! -f "$BACKUP_ARCHIVE" ]] block, as the trap handles them automatically.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/verify_backup_permissions.sh` around lines 2 - 9, The script sets -e
but doesn't register a cleanup trap, so temporary TEST_DIR may be left behind on
failures; add a cleanup function (e.g., cleanup()) that does a safe rm -rf
"$TEST_DIR" and register it with trap 'cleanup' EXIT (and optionally on ERR INT
TERM) after TEST_DIR is created, then remove any manual rm -rf "$TEST_DIR"
occurrences in the script and rely on the trap to perform teardown; ensure the
trap is installed after TEST_DIR is assigned so the cleanup function can
reference it, and keep TEST_DIR, BACKUP_DIR, SOURCE_DIR, CONFIG_FILE variable
names unchanged.

---

51-70: Stat detection logic is correct; minor stdout nit in the elif branch.

The %Lp format on BSD/macOS returns numeric octal permissions (e.g., 600), so comparisons against "600" / "400" / "700" are correct on both platforms. One nit: the elif stat --version 2>/dev/null; then on line 55 does not redirect stdout, so if this branch were ever reached, the version string would be printed to test output. Since this branch is dead code in practice, it's low priority.

♻️ Optional fix for the elif stdout

-elif stat --version 2>/dev/null; then
+elif stat --version >/dev/null 2>&1; then

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/verify_backup_permissions.sh` around lines 51 - 70, The elif branch
currently uses the test `elif stat --version 2>/dev/null; then` which leaves
stdout unredirected and may print the version string; update that conditional to
suppress output (e.g., redirect both stdout and stderr) so it closely mirrors
the GNU-detection branch, leaving the subsequent assignment to PERMS and
DIR_PERMS (the `PERMS=$(stat -c "%a" "$BACKUP_ARCHIVE")` and `DIR_PERMS=$(stat
-c "%a" "$BACKUP_DIR")` lines) unchanged.

---

28-39: Implicit yq dependency surfaces as a cryptic "Backup script failed" failure.

If yq is not installed, backup-projects.sh silently falls back to default folders (~/kidchenko, ~/lambda3, …), finds none, and exits 1. The test reports failure but only reveals the cause in the captured log — not ideal for CI diagnostics.

♻️ Add an explicit pre-check

 # Determine repo root
 REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
 BACKUP_SCRIPT="$REPO_ROOT/tools/backup-projects.sh"
+
+# Pre-flight: yq is required by backup-projects.sh to read the test config
+if ! command -v yq &>/dev/null; then
+    echo "SKIP: yq is not installed; cannot run backup permission tests."
+    rm -rf "$TEST_DIR"
+    exit 0   # or exit 77 if using an autotest framework that understands SKIP
+fi

 echo "Running backup script: $BACKUP_SCRIPT"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/verify_backup_permissions.sh` around lines 28 - 39, Add an explicit
pre-check for the yasnippet "yq" binary before invoking BACKUP_SCRIPT so CI
failures are immediately clear: in tests/verify_backup_permissions.sh, before
running bash "$BACKUP_SCRIPT" use a command existence check (e.g., command -v yq
or which yq) and if missing write a clear error both to stdout and to
"$TEST_DIR/backup.log" and exit 1; reference the existing symbols BACKUP_SCRIPT,
CONFIG_FILE and TEST_DIR so the check is colocated with the current backup
invocation and produces an actionable message like "yq not found; please install
yq" before attempting backup.

tools/backup-projects.sh (1)

353-361: chmod 700 additions are a good defense-in-depth layer.

Since umask 077 already ensures newly created directories start at mode 700, these chmod calls are primarily valuable for remediating directories that existed before this fix (e.g., previously created at 755). No issues.

One operational note: pre-existing *.zip archives already sitting in BACKUP_TEMP_DIR keep their original world-readable permissions. A one-time sweep during first run (or documented manual step) may be warranted to bring them in line.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/backup-projects.sh` around lines 353 - 361, Keep the chmod 700 calls
for BACKUP_TEMP_DIR and LOG_DIR inside the DRY_RUN check (when DRY_RUN != true)
and add a one-time remediation sweep that, only when not a dry run, tightens
permissions on existing backup archives in BACKUP_TEMP_DIR (e.g., change
world-readable zip files to owner-only) so pre-existing *.zip files aren’t left
world-readable; reference the DRY_RUN branching around BACKUP_TEMP_DIR, LOG_DIR
and the debug path, and perform the sweep only in the non-dry-run branch on
first-run or via a documented one-time step.